🦋 Changeset detected

Latest commit: 7f96331

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Prior to me examining this further:

• See Perf: Avoid creation of intermediary array when iterating over style rules #1272 for an example of where second argument of Array.from was introduced for performance reasons.
• See Guard against redefinition of Date.now #1196 for a similar guard against an old 3rd party js library which in that case defined it's own Date.now (before there was a standard Date.now)

Having had a quick look, we'll need something better than reverting #1272 which is kind of what this is ...

• Could make our own definition of array_from which checks whether it's safe to use the inbuilt one first
• Would also need to check whether our own one doesn't slow things down
• There's some trick I remember where you create a new 'clean' document context, pull out the native Array.from function from that context, and re-overwrite the one in the top level context (which has been doctored by prototypejs)

Notwithstanding #1196, I think that's more suitable for code outside of rrweb, as prototype.js is old and doesn't do this anymore right?

@eoghanmurray that is all very fair hence why I said we weren't sure if opening this PR is really worth it and wanted to get your opinion. Ultimately we at Pendo run rrweb in such a wide array of applications that are outside of our control and often are using old frameworks, weird polyfills, weird prototype changes, etc. and so have to find ways to keep rrweb stable in such hostile environments. This is one such change that we had made that worked for us and I would love to find an easy to not keep such a silly change in our fork but just stick to upstream so ultimately opening the PR was to see if this sort of change or one similar that addresses the same issue makes sense to get upstream in any way.

Ultimately it is likely that another Array.from usage like this will find its way in (as shown in #1272) so our change here isn't very stable either without moving to rrweb doing some sort of local usage of array_from or native_arrayFrom type thing which as you stated does seem odd given it is in older tooling.

we'll need something better than reverting #1272 which is kind of what this is ...

This doesn't really equate to reverting #1272 at all however, since that was making it so that we only loop over the list once instead of once to create it into an array and then once to map it to a stringified value. This change still results in a single loop it just does it using a standard loop instead of Array.from's map parameter.

Notwithstanding #1196, I think that's more suitable for code outside of rrweb, as prototype.js is old and doesn't do this anymore right?

This is the bigger question. Prototype.js does still do this and anyone that uses prototype.js will be getting errors when they try to use rrweb. Yes prototype.js is old and not used very widely but it is still used sadly so it comes down to what is the best way to support people that use libraries like this I guess. We at Pendo will do what we have to do to support these types of applications and altering rrweb was the solution we came up with but maybe there is some better middle ground that doesn't require forcing rrweb to support prototype.js while making it easy to not break down on such applications.

Also "there isn't really a good reason or a sustainable way to do this in rrweb" is a totally reasonable final decision. This was a combination of me going through our current changes that have diverted from upstream and making sure I at least saw if there was a good way to do it.

Apologies I missed that the changes do not add a new array.

I knew I'd seen this problem before; here's what we do at Statcounter (before rrweb is run):

       // MooTools 1.4.5 overrides Array.from
       // Array.from with 2nd mapping argument is used by rrweb
       if (Array.from([1], (x)=>x*2)[0] !== 2) {
               // https://stackoverflow.com/a/8580576/6691
               const dummy_iframe = document.createElement("iframe");
               document.documentElement.appendChild(dummy_iframe);
               Array.from = dummy_iframe.contentWindow.Array.from;  // a clean working version
               //Array.prototype.from = _window.Array.prototype.from;  // this is given in SO answer but doesn't work for mootools
               document.documentElement.removeChild(dummy_iframe);
       }

This could conceivably be added to packages/rrweb/src/record/index.js - take a stab at it if you like!

@eoghanmurray good idea. I have done that with other tools before just for some reason didn't think about trying that here. Probably was trying to be as surgical as possible which might have made sense for a quick hotfix on our part but definitely doesn't make as much sense for the long term evolution of rrweb.

Thanks for working out the typing issues! I'll leave it to another to review as I won't have seen problems in the code I suggested.

Fwiw, I've debugged the exact same issue this week where a site had overridden the Array.from implementation. Would love to see protection against this but also have no problem telling people that the issue is on their end 😅